ui: Distinguish bot users in more places by sm-sayedi · Pull Request #764 · zulip/zulip-flutter

sm-sayedi · 2024-06-26T07:37:59Z

Now that #605 is merged already, bot icons are shown with the name of bot users in MessageListPage. Additional places where users appear are covered in this PR:

ProfilePage
RecentDmConversationsPage

ProfilePage

Before	After	After (with long name)

RecentDmConversationsPage

Before	After	After (with long name)

Note: In RecentDmConversationsPage I wasn't sure whether to add (and where to add) the bot icon to a group dm where there may be one or multiple bot recipients.

Fixes: #636

rajveermalviya

Thanks! Changes LGTM and tests well :)
Moving on to mentor review, cc @hackerkid.

hackerkid · 2024-06-27T22:31:47Z

          child: Center(
            // TODO(#95) need dark-theme color
            child: Icon(ZulipIcons.group_dm, color: Colors.black.withOpacity(0.5))));
+        isBot = false;


Why is this always false? Isn't it possible to create DMs with multiple bots?

In RecentDmConversationsPage, I wasn't sure whether to add (and where to add) the bot icon to a group dm where there may be one or multiple bot recipients.

As I mentioned in the PR description above, I wasn't quite sure about this. There are two cases in group DMs with bot users:

A group DM with both real and bot users; in which I think it is not reasonable to add a bot icon at the end of the label as we do it in 1:1 DM with bot user. One approach we can take is to add a bot icon after the name of each bot user, something like this: Welcome Bot 🤖, Mahmood, S Muhmood. Or we can ignore the bot icon at all. I am not sure.

A group DM with just bot users; in which I think we can add a bot icon at the end of the label, but again, it will be confused with the first case, is it that the group DM is with multiple real users and one last bot user or the group DM is with all bot users?! If we ignore the bot icon in the first case, then this cannot be confusing!

Let's also hear from @gnprice about this!

This is a question about how the UI should behave for users, so it's a good one to get feedback on from @alya .

The bot icon should be associated with a particular user, not with a conversation as a whole. So I would expect it to appear after each bot's name, regardless of how many bot and non-bot users there are in the conversation.

By the way, zulip/zulip#26831 is the corresponding web app issue.

That makes a lot of sense. Thanks for the clarification @alya!

gnprice · 2024-06-28T16:52:28Z

The bot icon should be associated with a particular user, not with a conversation as a whole. So I would expect it to appear after each bot's name

@sm-sayedi For how to implement this behavior, the key Flutter feature I think you'll want is WidgetSpan. For example in message content that's how we handle custom emoji, user mentions, and global times.

… text In the next commit, the title of recent-dms item is changed from "Text" to "Text.rich", so this commit will make things ready for that change.

Fixes: zulip#636

sm-sayedi · 2024-07-03T10:16:44Z

@hackerkid, @alya Revision pushed with images also updated in the PR description. PTAL.

alya · 2024-07-03T22:03:55Z

We would ideally show the bot icon even if the name gets abbreviated.

gnprice · 2024-07-08T20:04:41Z

Advancing to maintainer review, since mentor review is complete.

In general reviewers should advance the PR to the next stage when they're done reviewing (or done except a few small comments). It's also a good idea for PR authors to check in to see if that hasn't happened, and advance it themselves.

For cross-reference, there's a chat thread started from Alya's comment above:
https://chat.zulip.org/#narrow/stream/48-mobile/topic/bot.20icon.20in.20recent-dms.20items.20-.20.23F764/near/1846941

That doesn't affect the first commit in this PR, though. Not sure if it'll affect the later commits enough to hold off reviewing them until it's been revised for that.

chrisbobbe

Thanks @sm-sayedi! Comments below.

chrisbobbe · 2024-07-08T21:55:13Z

-          matching: find.text(expectedText),
+          matching: find.byWidgetPredicate((widget) => widget is Text
+            && (widget.textSpan?.toPlainText(includePlaceholders: false) ?? '') == expectedText),


recent-dms test [nfc]: Change the checkTitle function to match rich text

I'm getting some test failures at this commit, but they go away in the next commit. I think it's fine to squash the test change in with the change that fixes the failures. It's not uncommon (though can be helpful to avoid when possible) that a test ends up depending on some implementation detail, and needs to be updated when that detail changes.

chrisbobbe · 2024-07-08T22:04:26Z

-                title))),
+                TextSpan(children: List.generate(users.length, (index) {
+                  final user = users[index];
+                  return TextSpan(text: user.name, children: [
+                    if (user.isBot) ...[
+                      const WidgetSpan(child: SizedBox(width: 5)),
+                      const WidgetSpan(
+                        child: Icon(ZulipIcons.bot, size: 15,
+                          color: Color.fromARGB(255, 159, 173, 173)),
+                        alignment: PlaceholderAlignment.middle),
+                    ],
+                    if (index < users.length - 1) const TextSpan(text: ', '),
+                  ]);
+                }).toList())))),


I see a comment on the switch:

// TODO dedupe with DM items in [InboxPage]

and that reminds me that we also show users' names in the Inbox page, when there are unread DMs. Would you add a prep commit that makes a helper function that can be used in this page and the Inbox page too? Perhaps it can return a TextSpan, and it can live in either file or perhaps a new file.

chrisbobbe · 2024-07-08T22:27:50Z

+          testWidgets('bot recipient -> shows no bot icon', (tester) async {
+            await checkRecipient(tester, isBot: true, looksBot: false);
+          });
+
+          testWidgets('non-bot recipient -> shows no bot icon', (tester) async {
+            await checkRecipient(tester, isBot: false, looksBot: false);
+          });


group('no error when user somehow missing from store.users', () { // […] testWidgets('bot recipient -> shows no bot icon', (tester) async { await checkRecipient(tester, isBot: true, looksBot: false); }); testWidgets('non-bot recipient -> shows no bot icon', (tester) async { await checkRecipient(tester, isBot: false, looksBot: false); });

The "bot recipient" test doesn't actually simulate a bot recipient, right, because these tests are checking what happens when the user data is missing. I see that the User passed to eg.dmMessage has isBot: true, but the isBot doesn't actually get written into the DmMessage object, so the app code can't treat this case differently than the "non-bot recipient" case.

Simplest I think to leave this test is it was, but with an added checkBotIcon(count: 0);.

chrisbobbe · 2024-07-08T22:29:29Z

+          testWidgets('bot recipient -> shows no bot icon', (WidgetTester tester) async {
+            await checkRecipient(tester, isBot: true, looksBot: false);
+          });


We would like the bot icon to appear even when the name is long; see Alya's comment: #764 (comment)

If that's hard to do and has to be left as a TODO, that might be OK, but we shouldn't have a test that confirms the undesired behavior. 🙂

chrisbobbe · 2024-07-08T22:33:17Z

-      Text(user.fullName,
-        textAlign: TextAlign.center,
-        style: _TextStyles.primaryFieldText
-          .merge(weightVariableTextStyle(context, wght: 700))),
+      Row(
+        mainAxisAlignment: MainAxisAlignment.center,
+        children: [
+          if (user.isBot) ...[
+            const Icon(
+              ZulipIcons.bot,
+              size: 22,
+              color: Color.fromARGB(255, 159, 173, 173),
+            ),
+            const SizedBox(width: 10),
+          ],
+          Flexible(child: Text(user.fullName,
+            textAlign: TextAlign.center,
+            style: _TextStyles.primaryFieldText
+              .merge(weightVariableTextStyle(context, wght: 700)))),
+        ],
+      ),


I think it would look better to style this like we do in the other places: first the name, then the bot icon, as an inline WidgetSpan. Is the style in this revision based on something you'd like me to look at?

chrisbobbe · 2024-07-08T22:36:33Z

+            const Icon(
+              ZulipIcons.bot,
+              size: 22,
+              color: Color.fromARGB(255, 159, 173, 173),


Let's put this in the new DesignVariables, and use the new variable here and in the message list (where we added the bot icon for senders in #605).

gnprice · 2024-08-01T00:52:34Z

Thanks @sm-sayedi for your work on this, and @hackerkid @chrisbobbe for the reviews!

After reviewing our priorities over the past week, this issue #636 is one I'm content to leave open past the launch of the app. So let's leave this PR for it aside; it'll serve as a basis to start from when someone comes back later, after launch, to pick up the issue and finish it. I'll close the PR to get it out of our dashboards since we won't actively be working on it, but it'll remain linked from the issue so easy to find when the time comes.

In the meantime, we have plenty of other issues to focus our energy on 🙂

sm-sayedi force-pushed the issue-636-distinguish-bot-users-in-more-places branch from 67ca590 to b3cf973 Compare June 26, 2024 07:46

sm-sayedi added the buddy review GSoC buddy review needed. label Jun 26, 2024

sm-sayedi requested a review from rajveermalviya June 26, 2024 07:54

sm-sayedi force-pushed the issue-636-distinguish-bot-users-in-more-places branch 2 times, most recently from 1a2633f to 7e72578 Compare June 27, 2024 08:14

rajveermalviya approved these changes Jun 27, 2024

View reviewed changes

rajveermalviya added the mentor review GSoC mentor review needed. label Jun 27, 2024

rajveermalviya requested a review from hackerkid June 27, 2024 17:22

rajveermalviya removed the buddy review GSoC buddy review needed. label Jun 27, 2024

hackerkid reviewed Jun 27, 2024

View reviewed changes

sm-sayedi added 3 commits July 3, 2024 14:28

profile: Distinguish isBot: true users with a bot marker

7766d1d

recent-dms test [nfc]: Change the checkTitle function to match rich…

930e269

… text In the next commit, the title of recent-dms item is changed from "Text" to "Text.rich", so this commit will make things ready for that change.

recent-dms: Distinguish isBot: true users with a bot marker

5ef3c62

Fixes: zulip#636

sm-sayedi force-pushed the issue-636-distinguish-bot-users-in-more-places branch from 7e72578 to 5ef3c62 Compare July 3, 2024 09:59

sm-sayedi requested review from alya and hackerkid July 3, 2024 10:16

hackerkid approved these changes Jul 3, 2024

View reviewed changes

gnprice added the maintainer review PR ready for review by Zulip maintainers label Jul 8, 2024

gnprice requested a review from chrisbobbe July 8, 2024 20:00

gnprice removed the mentor review GSoC mentor review needed. label Jul 8, 2024

chrisbobbe reviewed Jul 8, 2024

View reviewed changes

gnprice closed this Aug 1, 2024

Conversation

sm-sayedi commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ProfilePage

RecentDmConversationsPage

Uh oh!

rajveermalviya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Jun 28, 2024

Uh oh!

sm-sayedi commented Jul 3, 2024

Uh oh!

alya commented Jul 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented Jul 8, 2024

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Aug 1, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

sm-sayedi commented Jun 26, 2024 •

edited

Loading

alya commented Jul 3, 2024 •

edited

Loading